Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use FORCE_COLOR=0 for all tests #6585

Merged
merged 1 commit into from
Oct 21, 2018
Merged

Use FORCE_COLOR=0 for all tests #6585

merged 1 commit into from
Oct 21, 2018
+127 −79

Conversation

hron
Copy link
Contributor

@hron hron commented Jun 30, 2018

Summary

This change ensures that the tests work regardless of the value of
FORCE_COLOR environment variable. There are environments where this
variable is set to 1 and the tests fail. For example the tests fail when
they are run from Jetbrains IDEs (Rubymine, Webstorm and etc).

istanbul-* packages were updated because 1.* version uses old support-colors package that doesn't allow to disable colors with FORCE_COLOR=0. Instead it interpreters zero value in the same way as 1 or anything else by forcing colors. The latest version of istanbul uses the updated package that disables colors if FORCE_COLOR=0

I suppose this patch is the logical final for #5646 and #5652.

Test plan

If you run tests on master with defined FORCE_COLOR=1 you will receive this:

PS E:\src\jest> $Env:FORCE_COLOR=1; yarn jest
...skip...
Test Suites: 45 failed, 234 passed, 279 total
Tests:       131 failed, 91 skipped, 2599 passed, 2821 total
Snapshots:   15 failed, 10 obsolete, 982 passed, 997 total
Time:        537.5s, estimated 563s

With this patch applied the situation is resolved and all tests pass regardless of the value of FORCE_COLOR.

@hron hron force-pushed the set-force-color-0-in-tests branch from 7787691 to c39fadb Compare June 30, 2018 18:49
@SimenB
Copy link
Member

SimenB commented Jul 1, 2018

This pulls in a lot of babel 7 stuff, I don't think we can do that (see #6562).

Can we run strip-ansi on all coverage output or something to avoid the dependency bump?

@hron hron force-pushed the set-force-color-0-in-tests branch 2 times, most recently from 6c6768d to 461e278 Compare July 1, 2018 12:55
@hron
Copy link
Contributor Author

hron commented Jul 1, 2018

@SimenB Okay! I've just updated the patch to avoid any dependency bumps. I used strip-ansi as you suggested, but it was added in runJest to avoid repetition. Let me know if it's a bad idea!

@orta
Copy link
Member

orta commented Jul 1, 2018

If you rebase, it /should/ fix your CI

@SimenB
Copy link
Member

SimenB commented Oct 20, 2018

@hron could you rebase?

@hron
Use FORCE_COLOR=0 for all tests
48574de 
This change ensures that the tests work regardless of the value of
FORCE_COLOR environment variable. There are environments where this
variable is set to 1 and the tests fail. For example the tests fail when
they are run from Jetbrains IDEs (Webstorm, Rubymine and etc).
@hron hron force-pushed the set-force-color-0-in-tests branch from 803f045 to 48574de Compare October 21, 2018 15:20
@codecov-io
Copy link

Codecov Report

Merging #6585 into master will decrease coverage by 3.53%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6585      +/-   ##
==========================================
- Coverage   67.27%   63.73%   -3.54%     
==========================================
  Files         248      235      -13     
  Lines        9652     8935     -717     
  Branches        3        3              
==========================================
- Hits         6493     5695     -798     
- Misses       3158     3239      +81     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-validate/src/warnings.js 54.54% <0%> (-45.46%) ⬇️
packages/jest-validate/src/errors.js 54.54% <0%> (-45.46%) ⬇️
packages/jest-runtime/src/helpers.js 50% <0%> (-40.48%) ⬇️
packages/jest-runner/src/index.js 47.82% <0%> (-27.18%) ⬇️
packages/jest-editor-support/src/Process.js 66.66% <0%> (-26.67%) ⬇️
packages/jest-each/src/index.js 77.77% <0%> (-22.23%) ⬇️
packages/jest-haste-map/src/worker.js 73.33% <0%> (-20.96%) ⬇️
packages/jest-docblock/src/index.js 80% <0%> (-20%) ⬇️
packages/jest-leak-detector/src/index.js 66.66% <0%> (-16.67%) ⬇️
packages/jest-editor-support/src/Settings.js 71.05% <0%> (-15.62%) ⬇️
... and 176 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e9818dc...48574de. Read the comment docs.

@SimenB SimenB merged commit 8c6dcc7 into jestjs:master Oct 21, 2018
@SimenB
Copy link
Member

SimenB commented Oct 21, 2018

Thanks!

thymikee
thymikee reviewed Oct 21, 2018
View reviewed changes
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thymikee thymikee thymikee left review comments

@SimenB SimenB SimenB approved these changes

Assignees
No one assigned
Labels
cla signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hron @SimenB @orta @codecov-io @thymikee @facebook-github-bot